Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Decouple connection from Blob #823

Merged
merged 5 commits into from
Apr 16, 2015
Merged

Decouple connection from Blob #823

merged 5 commits into from
Apr 16, 2015

Conversation

dhermes
Copy link
Contributor

@dhermes dhermes commented Apr 14, 2015

NOTE: Has #822 as diffbase.

Towards #728

@tseaver I made a separate commit for each method with used Blob.connection but can combine them if you like.

I wanted to get rid of the Blob.connection property, but _PropertyMixin.reload() and _PropertyMixin.patch() use it as well as ObjectACL.reload() and ObjectACL.save().

@dhermes dhermes added the api: storage Issues related to the Cloud Storage API. label Apr 14, 2015
@googlebot googlebot added the cla: yes This human has signed the Contributor License Agreement. label Apr 14, 2015
@dhermes dhermes changed the title Decouple connection in blob Decouple connection from Blob Apr 14, 2015
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 5cf9023 on dhermes:decouple-connection-in-blob into 6283ab9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 15, 2015

9932f0d LGTM
e429f9a LGTM
8ebb280 see comments inline (setting connection=None in ctors to verify that override is used)
824ff20 same issue
5cf9023 same issue

@dhermes dhermes force-pushed the decouple-connection-in-blob branch from 5cf9023 to a54b630 Compare April 15, 2015 21:50
@dhermes
Copy link
Contributor Author

dhermes commented Apr 15, 2015

@tseaver I imagine this being a transient issue since at some point connection will not be bound to any of the types or to the stubs.

I'm not sure what would need to be checked either.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

@dhermes in the tests, we should pass None to the bucket ctor in order to ensure that the passed-in connection is being used (the tests as we have them would pass if the methods took the connection argument, but then fell back to using self.connection).

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

The heisenbug that @jgeewax fixed in #834 raised it's head here too. Only the 2nd time I had seen (build failure was first). Will rebase.

@tseaver All the Blob methods I changed use the passed in connection only and don't fall back to self.connection.

They are moving towards no connection anywhere, so falling back is a no-no.

@dhermes dhermes force-pushed the decouple-connection-in-blob branch from a54b630 to ca34a4a Compare April 16, 2015 16:30
@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling ca34a4a on dhermes:decouple-connection-in-blob into 40d84e9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

That's why having the bucket initialized with an invalid connection is the Right Thing (TM): it ensures that fallback cannot cause the test to succeed.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Gotcher. Current plan is to cherry-pick each of the 5 commits and then replace Bucket(connection) with Bucket(None) or similar. (About to spend an hour without internet on BART.)

LMK if this is a bad idea.

@dhermes
Moving _require_connection from storage.api to helpers.
e5a72fd 
This is to blob and bucket can use it without import cycles.
@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

SGTM

@dhermes
Removing using of Blob.connection in generate_signed_url.
e3ccb8d 
Allowing the default connection to be used or direct use of
a credentials object.
@dhermes
Removing using of Blob.connection in Blob.exists().
0421117 
Allowing the default connection to be used.
@dhermes
Removing using of Blob.connection in all Blob.download* methods.
970beaa 
Allowing the default connection to be used as fallback or takes
an explicit connection argument.
@dhermes
Removing using of Blob.connection in all Blob.upload* methods.
60d43ac 
Allowing the default connection to be used as fallback or takes
an explicit connection argument.
@dhermes dhermes force-pushed the decouple-connection-in-blob branch from ca34a4a to 60d43ac Compare April 16, 2015 18:24
@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

@tseaver PTAL

@coveralls
Copy link

Coverage Status

Coverage remained the same at 100.0% when pulling 60d43ac on dhermes:decouple-connection-in-blob into 40d84e9 on GoogleCloudPlatform:master.

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

LGTM

dhermes added a commit that referenced this pull request Apr 16, 2015
@dhermes
Merge pull request #823 from dhermes/decouple-connection-in-blob
06e26ad 
Decouple connection from Blob
@dhermes dhermes merged commit 06e26ad into googleapis:master Apr 16, 2015
@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

@tseaver Can we divvy up work and try to close out #728 together?

@dhermes dhermes deleted the decouple-connection-in-blob branch April 16, 2015 19:31
@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

How do you see the split?

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Can you handle the methods in acl.py and I'll try to cover the rest (Bucket and _PropertyMixin)?

Then we "converge" and remove the connection property from Blob (it gets used in acl.py).

@tseaver
Copy link
Contributor

tseaver commented Apr 16, 2015

OK. I just inventoried those methods in #825.

@dhermes
Copy link
Contributor Author

dhermes commented Apr 16, 2015

Awesome! Thanks

@dhermes dhermes mentioned this pull request Jun 4, 2015
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
api: storage Issues related to the Cloud Storage API. cla: yes This human has signed the Contributor License Agreement.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@dhermes @coveralls @tseaver @googlebot